👋 Welcome back sgehwolf! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@jerboaa This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8372155: ReleaseInfoPlugin doesn't handle input file as UTF-8 properly

Reviewed-by: alanb, jpai

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 75 new commits pushed to the master branch:

• a091af1: 8371475: HttpClient: Implement CUBIC congestion controller
• c12389d: 8372359: Clean jpackage error messages
• aec5472: 8372159: HttpClient SelectorManager thread could be a VirtualThread
• ... and 72 more: https://git.openjdk.org/jdk/compare/99135d2e05bb501fe9f9f0d36abd25894d0f93de...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@jerboaa The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Webrevs

• 04: Full - Incremental (b05d1f68)
• 03: Full - Incremental (65e57118)
• 02: Full - Incremental (580b4b13)
• 01: Full - Incremental (2f48c81b)
• 00: Full (2059e527)

Hello Severin, would it better to (even) specify the expectations of the --release-info jlink plugin and what encoding it expects for the file content?

I might be wrong, but even with the use of UTF-8 for reading the given file (like proposed in this PR), it may not guarantee that it is the right Charset to use for that file.

Thanks for the review.

Hello Severin, would it better to (even) specify the expectations of the --release-info jlink plugin and what encoding it expects for the file content?

IIUYC, you'd like to have the help text amended for --release-info to say it expects the input file in UTF-8? Is that what you are saying?

IMO it would be best for --release-info to not only require a file as an option but also the encoding. It could perhaps default to UTF-8. @AlanBateman Any thoughts on this?

I might be wrong, but even with the use of UTF-8 for reading the given file (like proposed in this PR), it may not guarantee that it is the right Charset to use for that file.

True. If somebody passes an ISO-8859-1 (or any non-utf8) encoded file to --release-info it would break for any non-ASCII characters.

Hello Severin, would it better to (even) specify the expectations of the --release-info jlink plugin and what encoding it expects for the file content?

IIUYC, you'd like to have the help text amended for --release-info to say it expects the input file in UTF-8? Is that what you are saying?

Correct, but not necessarily UTF-8. I am not familiar where all this release-info gets used during jlink and then in the generated image. So whatever encoding is appropriate for such content, I think we should specify it, in the help text and man page of jlink.

Correct, but not necessarily UTF-8.

It expects the input file to be UTF-8 now. So I've added that the plugin help text in 2f48c81

I am not familiar where all this release-info gets used during jlink and then in the generated image.

It's the plugin that creates the $JDK/release file. That's it.

So whatever encoding is appropriate for such content, I think we should specify it, in the help text and man page of jlink.

I've added it to the --list-plugin help text now. Plugins aren't mentioned in the jlink man page, so it doesn't seem appropriate to mention it the jlink man page. Here is how it looks like when using jlink --list-plugins with the latest update:

  --release-info <file>|add:<key1>=<value1>:<key2>=<value2>:...|del:<key list>
                            <file> option is to load release properties from
                            the supplied file. The specified file is expected
                            to be encoded in UTF-8.
                            add: is to add properties to the release file.
                            Any number of <key>=<value> pairs can be passed.
                            del: is to delete the list of keys in release file.

IMO it would be best for --release-info to not only require a file as an option but also the encoding. It could perhaps default to UTF-8. @AlanBateman Any thoughts on this?

It's very likely that tools that read the release file aren't using Properties API so I think having it UTF-8 is best.

IMO it would be best for --release-info to not only require a file as an option but also the encoding. It could perhaps default to UTF-8. @AlanBateman Any thoughts on this?

It's very likely that tools that read the release file aren't using Properties API so I think having it UTF-8 is best.

The output $JDK/release file will be UTF-8 since the ReleaseInfoPlugin uses PrintWriter(BytearrayOutputStream) to write the content bytes to the ResourcePoolEntry. That in-turn uses UTF-8 since JEP 400 (by means of Charset.defaultCharset().

Question is if it's worth supporting arbitrary input encondings for --release-info <file> usages. <file> is currently in UTF-8 in the JDK build, so went with that expectation. But arguably it could be any other encoding a user chooses. So to make it generic, it's conceivable to allow --release-info=<file>,<encoding> or some such to allow non-UTF-8 as input and still do the right thing (Note: output would still be UTF-8). Perhaps it's not worth the trouble?

Question is if it's worth supporting arbitrary input encondings for --release-info <file> usages. <file> is currently in UTF-8 in the JDK build, so went with that expectation. But arguably it could be any other encoding a user chooses. So to make it generic, it's conceivable to allow --release-info=<file>,<encoding> or some such to allow non-UTF-8 as input and still do the right thing (Note: output would still be UTF-8). Perhaps it's not worth the trouble?

That seems unnecessary complexity, I think we should just keep it as UTF-8.

Please fix the typo in the PR title. (I corrected the issue title).

Any more input on this one? I'd appreciate a review. Thanks!

Hello Severin, the changes this PR looks good to me. Should anything be specified/noted for the encoding of the key/value of the release-info plugin?

--release-info <key1>=<value1>

Should anything be specified/noted for the encoding of the key/value of the release-info plugin?

--release-info <key1>=<value1>

I don't think so. In my testing on Linux with en_US.UTF-8 LANG those are working fine:

$ ./jdk/bin/jlink --add-modules java.base --output ./build/testme --release-info 'add:myval=ööaßÄ oy'
$ grep myval ./build/testme/release 
myval=ööaßÄ oy
$ locale | grep LANG
LANG=en_US.UTF-8

/integrate

Going to push as commit 42b108b.
Since your change was applied there have been 81 commits pushed to the master branch:

• e00dec5: 8371667: Shenandoah: Re-design alloc request type enum for better efficiency and cleaner code
• 507a6d3: 8368001: java/text/Format/NumberFormat/NumberRoundTrip.java timed out
• 8bafc2f: 8371789: C2: More explicit dump results for TypePtr
• ... and 78 more: https://git.openjdk.org/jdk/compare/99135d2e05bb501fe9f9f0d36abd25894d0f93de...master

Your commit was automatically rebased without conflicts.

@jerboaa Pushed as commit 42b108b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.